-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add linode_database_postgresql_v2 resource #1680
Conversation
7b9f0ee
to
b90bf13
Compare
db, err := client.GetPostgresDatabase(ctx, id) | ||
if err != nil { | ||
if lerr, ok := err.(*linodego.Error); ok && lerr.Code == 404 { | ||
resp.Diagnostics.AddWarning( | ||
"Database no longer exists", | ||
fmt.Sprintf( | ||
"Removing PostgreSQL database with ID %v from state because it no longer exists", | ||
id, | ||
), | ||
) | ||
resp.State.RemoveResource(ctx) | ||
return | ||
} | ||
resp.Diagnostics.AddError( | ||
"Unable to refresh the Database", | ||
err.Error(), | ||
) | ||
return | ||
} | ||
|
||
resp.Diagnostics.Append(data.Refresh(ctx, client, db.ID, false)...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably a good way to eliminate the redundant network request here but I can't think of one. If anyone has any ideas please let me know!
@@ -0,0 +1,407 @@ | |||
//go:build integration || databasepostgresqlv2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests can take up to 30 minutes to run due to the need for databases to be updated, resized, forked, etc.
We should discuss ways to reduce the impact this has on our test suite runtime.
e514c89
to
bb0f69e
Compare
go.mod
Outdated
@@ -125,3 +123,5 @@ require ( | |||
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect | |||
sigs.k8s.io/yaml v1.3.0 // indirect | |||
) | |||
|
|||
replace github.com/linode/linodego => github.com/ezilber-akamai/linodego v0.0.0-20241202200116-8acd7eb1ae45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Remove this before merging
@@ -0,0 +1,41 @@ | |||
package unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we already have a package for unit test utils, and if not is there a better place for this package to live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be fine to leave this here for now. It will be good to look through the repository at some point to evaluate unit test coverage and possible improvements in a separate ticket.
@@ -50,7 +50,7 @@ jobs: | |||
echo "LINODE_TOKEN=${{ secrets.LINODE_TOKEN_USER_2 }}" >> $GITHUB_ENV | |||
;; | |||
"USER_3") | |||
echo "TEST_TAGS=instanceconfig,instancedisk,instanceip,networkingip,objcluster,objkey,profile,rdns,region,regions,stackscript,stackscripts" >> $GITHUB_ENV | |||
echo "TEST_TAGS=databasepostgresqlv2,instanceconfig,instancedisk,instanceip,networkingip,objcluster,objkey,profile,rdns,region,regions,stackscript,stackscripts" >> $GITHUB_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a pretty arbitrary decision, let me know if there's a better matrix user to run this under 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good decision since USER_3 takes the least time to finish usually. In future, as test coverage grows for database v2, maybe we can separate and dedicate a new test account
abc6f4a
to
0cead75
Compare
go 1.22.0 | ||
|
||
toolchain go1.22.5 | ||
go 1.23 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to use the iterator helpers introduced in this PR
LGTM, manual steps and tests all pass consistently. Great work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration and manual tests work great locally. Nice work!
📝 Description
This pull request implements a new
linode_database_postgresql_v2
resource supporting the new Aiven-backed managed database clusters.Design Caveats
Flattened Fork Object
The
fork
block has been flattened into root-levelfork_source
andfork_restore_time
attributes because:a. Additionally, treating it as a block would have inconsistent usage with the
updates
object.restore_time
is treated as optional and computed as an API, there is a valid case where the user does not want to specify it explicitly but does want to access the computed restore time.Independent Pending Updates Set
The
updates.pending
set has been split into a root-levelpending_updates
set, which allows users to specify their updates configuration as a single object.Flattened Hosts and Credentials Blocks
The
hosts
and credentials blocks have been flattened into root-level fields to simplify our implementation and to make unknown/null behavior a bit more predictable.✔️ How to Test
The following test steps assume you have pulled down this PR locally and do not have your account configured for legacy DBaaS clusters.
Unit Testing
Integration Testing
Manual Testing
NOTE: Database creations should take ~15 minutes.
allow_list
,cluster_size
,type
, andupdates
attributes.